Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: ExecAsPkg #644

Closed
wants to merge 3 commits into from
Closed

feat: ExecAsPkg #644

wants to merge 3 commits into from

Conversation

albttx
Copy link
Member

@albttx albttx commented Mar 24, 2023

With @tbruyelle we continue @moul 's works on #335 to add an ExecAsPkg function.

This solution will be able to solve the issue on #634

Description

We add a new OpCall for setting the m *Machine context, this allow to update the context

How has this been tested?

see tests/files/zrealm_std6.gno

Signed-off-by: Manfred Touron <94029+moul@users.noreply.github.com>
@albttx albttx requested a review from a team as a code owner March 24, 2023 14:14
@albttx albttx force-pushed the albttx/exec-as-pkg branch 3 times, most recently from a180ce1 to 56e494a Compare March 24, 2023 14:17
@@ -275,6 +275,48 @@ func InjectPackage(store gno.Store, pn *gno.PackageNode) {
m.PushValue(res0)
},
)
pn.DefineNative("ExecAsPkg",
// TODO: rename UnsafeExecAsPkg?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still uncertain, so I'd appreciate additional input. However, it appears that we have three primary options.

  • std.ExecAsPkg
  • std.UnsafeExecAsPkg
  • unsafe.ExecAsPkg

I believe it is essential to reassess our understanding of safety and risk when drafting contracts. After all, isn't the default assumption that everything is potentially unsafe?

Copy link
Member Author

@albttx albttx Mar 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I havn't removed your comments.

But i think we should keep std.ExecAsPkg, it's a "normal feature" for a smart-contract engine to be able to do actions as the smart-contract addr

The use of the word Unsafe could scare contract writers, and make them think as a bad practice.

Copy link
Contributor

@piux2 piux2 Mar 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I havn't removed your comments.

But i think we should keep std.ExecAsPkg, it's a "normal feature" for a smart-contract engine to be able to do actions as the smart-contract addr

The use of the word Unsafe could scare contract writers, and make them think as a bad practice.

@albttx

Gnolang support pass function as parameters and interface callback. It does exactly what current std.ExecAsPkg's implementation tries to do, and is more flexible and safer.

Not sure if std.ExecAsPkg implementation does type checking on the function parameters.

pn.DefineNative("ExecAsPkg",
// TODO: rename UnsafeExecAsPkg?
gno.Flds( // params
"fn", gno.FuncT(nil, nil),
),

"fn", gno.FuncT(nil, nil),

On the other hand, Gnolang, as in go, supports the passing functions as parameters does type checking of the passed functions' parameter types

To solve #634, as you mentioned, you can modify GRC20 with interface callback or pass a callback function as a parameter to allow the airdrop contract to alter the state of your token contract. In addition, you can add a static allowed list or even try to verify interface types for approval.

That is more idiomatic in go programming, safe and auditable.

Copy link
Contributor

@piux2 piux2 Mar 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// g157y5v3k529jyzhjjz4fn49tzzhf4gess6v39xg

  • Testing case is not robust. It exposes a safety issue for the contract to use std.ExecAsPkg. All three lines should have output the same address without knowing call path context has been changed.

  • It sideloads the behavior of std.GetOrigCaller() and makes contracts hard to audit. The safety and security of smart contracts have very much to do with readability and auditability.

  • More importantly, It makes checking std.GetOrigCaller() lose its purpose when we call std.ExecAsPkg(anyFunction).

Here is why:

std.GetOrigCaller() suppose to get the first caller's address of the entire call path. Usually, it is end users who initiate a function call on a smart contract. In Ethereum, that is an external owned account (EOA). In gno, we do not separate it since the main package can also be the OrigCaller.

No matter where std.GetOrigCaller() is called in the call path; it should always return the same address.

However, std.ExecAsPkg changes the context of the call path in the middle. It makes std.GetOrigCaller() returns the address of the contract that std.ExecAsPkg is called in.

m.PushOp(gno.OpSetContext)

// Push an alternate context with OrigCaller=OrigPkgAddr, this will
// affect the function call above
m.PushOp(gno.OpSetContext)
ctx := m.Context.(ExecContext)
ctx.OrigCaller = ctx.OrigPkgAddr

It makes checking std.GetOrigCaller() lose its purpose, when we call std.ExecAsPkg( any function ) in the same contract.

Co-authored-by: Thomas Bruyelle <thomas.bruyelle@gmail.com>
m.PushOp(gno.OpSetContext)

cx := gno.Call(gno.FuncT(nil, nil))
m.PushValue(gno.TypedValue{})
Copy link
Contributor

@tbruyelle tbruyelle Mar 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are still unsure why we need to add this useless value in the values' stack.

The thing is, if we don't, the stack is truncated by one value (when PopFrameReset is invoked). In that case, the truncated value is the context value we pushed earlier, and doOpSetContext cannot work properly without it.

We'll try to figure out why this value is truncated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some investigation, in a normal context, this truncated value is the FuncValue that will be invoked. In this particular context (native call), the FuncValue comes from the native function parameter, so it's not in the values' stack.

Finally, instead of using OpCall, we should use OpPreCall that is the layer just above. This is done here
d59c320, and thankfully, we no longer need to push a placeholder value.

println(std.GetOrigCaller())
}

// Output:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need more tests for this feature: multiple levels; reused func pointers; touching a variable outside of the scope; panic; probably more.

also add comments and small code reorg
@moul
Copy link
Member

moul commented Mar 31, 2023

Please share your thoughts on PR #683.

@jaekwon
Copy link
Contributor

jaekwon commented Apr 20, 2023

Agreed with piux, and as commented in other related PRs, we don't want this kind of dynamism.
There must be a pattern that uses only pure Go/Gno to achieve what we ultimately want.
Please see #566 (comment).
Also see #715 (review).

@jaekwon jaekwon closed this Apr 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants